Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature gates for for and ? in consts #87237

Merged
merged 5 commits into from
Jul 30, 2021
Merged

Add feature gates for for and ? in consts #87237

merged 5 commits into from
Jul 30, 2021

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Jul 18, 2021

These operations seems relatively straightforward to support, and only seem to be blocked on impl const Trait.

I have included a working test for const_try, but const_for is currently unusable without reimplementing every single defaulted Iterator method, so I didn't do that.

(both features still need tracking issues before this is merged)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2021
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2021

but const_for is currently unusable without reimplementing every single defaulted Iterator method, so I didn't do that.

maybe #86857 helps here?

@jonas-schievink
Copy link
Contributor Author

In theory, yeah, this works:

#![feature(const_refs_to_cell)]
#![feature(const_mut_refs)]
#![feature(const_fn_trait_bound)]
#![feature(const_trait_impl)]

trait Iter: Copy + Sized {
    type Item: Copy;
    
    fn next(&mut self) -> Option<Self::Item>;
    
    #[default_method_body_is_const]
    fn count(mut self) -> usize {
        let mut count = 0;
        while self.next().is_some() {
            count += 1;
        }
        count
    }
}

#[derive(Copy, Clone)]
struct ConstIter {
    next: Option<u8>,
}

impl const Iter for ConstIter {
    type Item = u8;
    
    fn next(&mut self) -> Option<u8> {
        match self.next {
            Some(val) => {
                self.next = val.checked_add(1);
                Some(val)
            }
            None => None,
        }
    }
}

const _: () = {
    let c = ConstIter { next: Some(0) }.count();
    if c != 256 {
        [()][4];
    }
};

...but it requires a lot of feature gates and Copy bounds (since everything is generic) that wouldn't work on Iterator. Seems like there's still a bunch of work to do until iterators work in consts.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2021

Then I would say we don't add a const_for feature gate, we can start figuring out const iterators without it.

@jonas-schievink
Copy link
Contributor Author

But we'll need one eventually, right? Why not add it now?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2021

Yea. Just right now it doesn't really get us anything actionable. I guess as long as we have a canary test showing the behaviour of the feature gate it should be ok to have an unusable feature gate. Just thought it unusual.

@bors
Copy link
Contributor

bors commented Jul 27, 2021

☔ The latest upstream changes (presumably #83484) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 29, 2021

☔ The latest upstream changes (presumably #86998) made this pull request unmergeable. Please resolve the merge conflicts.

@usbalbin
Copy link
Contributor

#86853 is a PR which might have some overlap with the try-operator part of this. My goal there was to try and constify the most trivial conversion impls to enable basic usage with Option and Result (however that turned out to be blocked on some more work on const_trait_impl)

@usbalbin
Copy link
Contributor

Btw is #87576 a duplicate of #74935? :)

@jonas-schievink
Copy link
Contributor Author

Yes, these tracking issues are pretty hard to discover

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Comment on lines +1 to +5
error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
--> $DIR/const-for.rs:5:14
|
LL | for _ in 0..5 {}
| ^^^^
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should start printing the function's name in the diagnostic. Very uninformative this error XD

@oli-obk
Copy link
Contributor

oli-obk commented Jul 30, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 30, 2021

📌 Commit c5a29f9 has been approved by oli-obk

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 30, 2021
@bors
Copy link
Contributor

bors commented Jul 30, 2021

⌛ Testing commit c5a29f9 with merge 87dc824...

@bors
Copy link
Contributor

bors commented Jul 30, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 87dc824 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2021
@bors bors merged commit 87dc824 into rust-lang:master Jul 30, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 30, 2021
@jonas-schievink jonas-schievink deleted the const-for-and-try branch July 30, 2021 15:09
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2021
Constify ?-operator for Result and Option

Try to make `?`-operator usable in `const fn` with `Result` and `Option`, see rust-lang#74935 . Note that the try-operator itself was constified in rust-lang#87237.

TODO
* [x] Add tests for const T -> T conversions
* [x] cleanup commits
* [x] Remove `#![allow(incomplete_features)]`
* [?] Await decision in rust-lang#86808 - I'm not sure
* [x] Await support for parsing `~const` in bootstrapping compiler
* [x] Tracking issue(s)? - rust-lang#88674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants